Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix theme manager script #16386

Merged
merged 11 commits into from
Jul 3, 2024
Merged

Fix theme manager script #16386

merged 11 commits into from
Jul 3, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Jun 28, 2024

The PR #16356 moved some of the theme related scripts into OrchardCore.Themes but did not include all the theme reusable scripts.

Here is a summary of the improvements

  • The ResourceManifest used in the Theme module is now static.
  • Moved theme-toggler and theme constants into the Themes module to allow reusability.
  • Now TheAdmin and TheTheme theme are able to use dark/light theme resources.
  • We now have theme-head script which should be loaded early on to ensure to apply user's preferences early instead of waiting on the page to load.
  • The script theme-manager provides you a way to toggle theme selection.

/cc @mdameer

@MikeAlhayek MikeAlhayek requested a review from mdameer June 29, 2024 23:08
@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jun 29, 2024

@mdameer I updated some of my comments after addressed some of your comments and cleanup. So please check out my updated comments.

"Assets/js/theme-toggler.js"
],
"watch": [
"Assets/js/theme-toggler.js"
],
"output": "wwwroot/Scripts/theme-manager.js"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case where you separate the theme-head from the toggler, then I think theme-toggler.js will be more clear, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After cleanup the dependency, I thought theme-manager is better fit. Because I added dependency for the theme manager to the theme-head. So including the theme-manager will include the theme-head automatically. The reason I included them separately to ensure that the theme-head is loaded early on. So theme-manager would be fine.

@MikeAlhayek MikeAlhayek requested a review from Piedone July 2, 2024 17:42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this with a basic Blog setup now:

image

This is after a hard refresh and a gulp rebuild (even though that shouldn't be necessary).

Same with dark:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why icons are not found. I don't experience this issue locally. I don't think this issue is related to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the toolbox being unusable in dark mode one thing that #16356 is supposed to fix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes and it does. but something on your local machine that is not discovering the icons.svg file which is the icons. I don't get the same behavior on my end. Every thing seems to be fine. I just tested it again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not discovering the SVG because it's not there under OrchardCore.Resources/Scripts/trumbowyg/ui/icons.svg. It's correctly a 404.

Isn't this file what you see as being requested?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Piedone the icons.svg file is available https://github.com/OrchardCMS/OrchardCore/tree/main/src/OrchardCore.Modules/OrchardCore.Resources/wwwroot/Scripts/ui

There is no issue on my side either. Check this out:

The light theme:
image

The dark theme:

image

Copy link
Member

@Piedone Piedone Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that what I see being requested is OrchardCore.Resources/Scripts/trumbowyg/ui/icons.svg, under the trumbowyg folder, and not OrchardCore.Resources/Scripts/ui/icons.svg. But this gets messier by there being a second SVG under wwwroot/Styles/ui.

It loads the incorrect one because control arrives here:

image

The path to the Styles icon, what I'd expect to be actually used, is configured in two places, but this is not taking effect for me. This is because these two are only used if the editor for HtmlBody is configured as Trumbowyg instead of "Wysiwyg editor", what is the default. So, the toolbar will only work for you if you change the editor.

I think the fix is to move the SVG to the script, so the default detection will work (i.e. you're not forced to set $.trumbowyg.svgPath), and remove the leftover other SVG. It doesn't seem to be referenced anywhere (like in CSS) so this should be fine. I committed this, please check and merge the PR if OK. It also fixes that the SVG in the wwwroot folder wasn't actually automatically copied, just I suppose updated by hand.

Also, for me, the toolbar is light even on main when switching to the Trumbowyg editor:

image

But that's a mystery I'll solve later.

BTW something is broken with Resource Debug Mode, because even when it's force enabled, some, but not all, resources, including Trumbowyg, are loaded with their minified versions:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! good catch. this issue was already in main and no one reported it. I also noticed that dark mode in the WYSIWYG template in HtmlBodyPart was not changing as I was trying to follow your steps above. I fix that as well.

I can also confirm that changing the resource debug mode, bootstrap.min.css is still loaded instead of bootstrap.css the rest seems to be loading as expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #16401 about this.

@MikeAlhayek MikeAlhayek enabled auto-merge (squash) July 3, 2024 15:47
@MikeAlhayek MikeAlhayek merged commit 7b4120e into main Jul 3, 2024
11 checks passed
@MikeAlhayek MikeAlhayek deleted the ma/fix-theme-script branch July 3, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants